Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Parse newlines in JSON message as row separators. #6944

Merged
merged 11 commits into from
Aug 11, 2023

Conversation

johnnesky
Copy link
Member

@johnnesky johnnesky commented Apr 1, 2023

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Fixes #6809. Interprets newline characters in the JSON message as dummies a new subclass of Input called EndRowInput that force any following inputs to be rendered on a new row.

Proposed Changes

Behavior Before Change

Newline characters from the message are preserved, but render as regular spaces by the browser in a label field.

Behavior After Change

Newline characters are converted to special dummy end-row inputs that force the next input to be rendered on a new line. (Also, these dummies use the same alignment as the implicit last dummy.)

The common RenderInfo recognizes the new dummies and handles them appropriately.

Reason for Changes

This allows more customization of block layout, including putting inline inputs on multiple rows.

Test Coverage

Unit tests in block_json_test.js and block_test.js test token parsing and conversion to dummies. Rendering is not tested.

Documentation

Information about using newline characters in JSON messages, and the new "input_end_row" JSON input type, could be added to: https://developers.google.com/blockly/guides/create-custom-blocks/define-blocks

@johnnesky johnnesky requested a review from a team as a code owner April 1, 2023 02:12
@johnnesky johnnesky requested a review from BeksOmega April 1, 2023 02:12
@johnnesky johnnesky changed the title Parse newlines in JSON message as row separators. feat: Parse newlines in JSON message as row separators. Apr 1, 2023
@github-actions github-actions bot added the PR: feature Adds a feature label Apr 1, 2023
Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM! Mostly comments about renaming parameters :P

Wrt documentation: yes we should definitely document this. Could you file a docs issue for that (when you have time) and add it to our Q2 Github Project?

Also @maribethb the idea was that this doesn't force v10 (even though it's technically a breaking change in that it would change block rendering for people currently using \n) correct?

core/block.ts Outdated Show resolved Hide resolved
core/renderers/geras/info.ts Show resolved Hide resolved
tests/mocha/block_json_test.js Outdated Show resolved Hide resolved
core/utils/parsing.ts Show resolved Hide resolved
core/utils/parsing.ts Show resolved Hide resolved
core/utils/parsing.ts Outdated Show resolved Hide resolved
core/block.ts Outdated Show resolved Hide resolved
core/input.ts Outdated Show resolved Hide resolved
@BeksOmega
Copy link
Collaborator

BeksOmega commented Apr 3, 2023

Another more design-related question (possibly for @maribethb ) Have we considered adding some more generic extra options to inputs that would allow them to be rendered differently? E.g. https://github.com/google/blockly/pull/3219/files#diff-e01c94caccf98f950fdbe393516b2595473c2e1f04e9f302ec62ed66549e2886R41

I believe this would allow App Inventor to implement their indented inputs (which it seems they still want) relatively easily externally.

[Edit: Discussed this with Rachel and neither of us really like the solution of "just allow people to append extra data". So we either want to have this PR go in as-is, or we want to do some more designwork to see if there's a more structured way to support alternative rendering]

core/utils/parsing.ts Outdated Show resolved Hide resolved
@BeksOmega BeksOmega marked this pull request as draft April 19, 2023 15:23
@johnnesky
Copy link
Member Author

I've reimplemented this as a new subclass of Input, similar to DummyInput!

@johnnesky johnnesky marked this pull request as ready for review August 4, 2023 23:16
@github-actions github-actions bot added PR: feature Adds a feature and removed PR: feature Adds a feature labels Aug 4, 2023
@johnnesky
Copy link
Member Author

Created an issue to add documentation: #7360

@johnnesky
Copy link
Member Author

Added input_end_row to the block factory demo.

Copy link
Collaborator

@BeksOmega BeksOmega left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM! Just a few little things =)

core/inputs/end_row_input.ts Show resolved Hide resolved
core/renderers/common/info.ts Outdated Show resolved Hide resolved
core/renderers/geras/info.ts Outdated Show resolved Hide resolved
core/renderers/geras/info.ts Outdated Show resolved Hide resolved
"previousStatement": "Input",
"nextStatement": "Input",
"colour": 210,
"tooltip": "For adding fields at the end of a row with no " +
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Update tooltip.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the tooltips for both this and dummy inputs, please take a look.

tests/mocha/block_json_test.js Show resolved Hide resolved
core/block.ts Outdated Show resolved Hide resolved
@BeksOmega BeksOmega merged commit f246adb into google:develop Aug 11, 2023
7 checks passed
@johnnesky
Copy link
Member Author

Created google/blockly-samples#1854 to deprecate the renderer plugin that accomplished something similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse newlines in dummy inputs as row separators
2 participants